Refactoring `EuiKeyPadMenuItem` beta badge for improved accessibility by 1Copenut · Pull Request #5508 · elastic/eui

1Copenut · 2021-12-28T20:41:12Z

Summary

The EuiKeypadMenuItem had a focusable span inside the focusable button, used to trigger the tooltips. Axe-core was complaining about this nested focusable element, so I refactored it to move it outside the button as a sibling element. Closes #5290.

Moved the EuiBetaBadge outside the parent button
Updated the absolute positioning of the beta badge to match current
Added role="button" to beta badge span for more consistent SR read out
Added an aria-label to the beta badge span[role="button"] so it offers context to screen readers

Not sure this qualifies as a breaking change but it might. The refactoring of HTML was limited to EuiKeypadMenuItem in cases where the beta badge is present. Happy to flag it as a breaking change if reviewers feel it's warranted.

Checklist

* Moved the EuiBetaBadge outside the parent button * Updated the absolute positioning of the beta badge to match current * Added role="button" to beta badge span for more consistent SR read out

1Copenut

Added comments about two of the proposed changes.

1Copenut · 2021-12-28T20:45:53Z

src/components/badge/beta_badge/beta_badge.tsx

          title={title || label}
        >
-          <span tabIndex={0} className={classes} {...rest}>
+          <span tabIndex={0} className={classes} role="button" {...rest}>


I added this role="button" because Safari + VoiceOver wasn't consistently announcing the tooltip. Found this article Short note on aria-label, aria-labelledby, and aria-describedby by Leonie Watson that gave me a clue the span alone might not honor our ARIA markup consistently.

1Copenut · 2021-12-28T20:47:02Z

src/components/key_pad_menu/_key_pad_menu_item.scss

  }
 }
+
+.euiKeyPadMenuListItem {


Moved this out as a top-level element in the SCSS to ensure consistent absolute positioning and lessening risk of the cascade modifying something.

kibanamachine · 2021-12-28T21:22:33Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5508/

src/components/key_pad_menu/_key_pad_menu_item.scss

src/components/key_pad_menu/key_pad_menu.tsx

chandlerprall · 2021-12-29T17:29:03Z

src/components/key_pad_menu/key_pad_menu_item.tsx


    return (
      <EuiBetaBadge
+        aria-label={`${label}. Beta item.`}


Any hard coded text should use the useEuiI18n utility to allow localization.

@cchaos does this labelling match the intent of specifying an iconType?

My guess is that we should actually be labelling button as beta not the badge. And that we can’t hard-code “Beta item” at all, that the beta label needs to come from the betaBadgeLabel that the consumer supplies since it's not always going to mean "Beta" specifically.

@1Copenut I think we need to re-think this solution. Mainly, one of the issues I'm seeing is that if a consumer were to use EuiKeyPadMenuItem by itself, this beta badge would no longer be poisitioned against the button, but whatever next parent is position: relative.

Would it be a better solution to revert back the styling to include the beta badge within the button, but aria-hidden it and just apply the betaBadgeLabel as an extra aria-describedBy?

Thank you @cchaos for this use case my solution hadn't anticipated. ~~I'll give it another pass in a new branch with the approach you outlined and keep things in sync here so we don't lose the history.~~

I talked about this with Chandler in our 1:1 yesterday. Refactored it to add a relatively positioned DIV. I couldn't keep the beta badge nested inside the button because the axe plugin was complaining about a nested element, and hiding it with aria-hidden="true" would have removed the tooltip context for screen readers.

Moving the beta badge out to a sibling of the button felt like the best approach to balance all concerns.

src/components/key_pad_menu/key_pad_menu_item.tsx

kibanamachine · 2021-12-29T20:32:21Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5508/

chandlerprall · 2021-12-30T19:36:51Z

src/components/key_pad_menu/key_pad_menu_item.tsx

    },
    className
  );
+  const betaItemLabel = useEuiI18n('euiKeyPadMenuItem.betaLabel', 'Beta item.');


Sorry, I could've provided more guidance on the i18n util; including the label variable here will make the whole label localizable, instead of forcing the order.

Suggested change

const betaItemLabel = useEuiI18n('euiKeyPadMenuItem.betaLabel', 'Beta item.');

const betaItemLabel = useEuiI18n(

'euiKeyPadMenuItem.betaLabel',

'{label}. Beta item.',

{ label }

);

and then,

aria-label={betaItemLabel}

I'll grab this on Tuesday morning right away!

kibanamachine · 2022-01-04T16:44:17Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5508/

1Copenut · 2022-01-11T16:40:19Z

src/components/key_pad_menu/key_pad_menu_item.tsx

-    >
-      {renderContent()}
-    </Element>
+    <div className="euiKeyPadMenuItem__outer">


After talking with @chandlerprall on 1/10, I added a containing DIV to manage relative positioning inside the EuiKeypadMenuItem component. This addresses @cchaos concern that we might lose that reference if the component is used outside the original context, and maintains the fix to support mouse, keyboard, and screen reader (semantic) accessibility.

kibanamachine · 2022-01-11T17:11:42Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5508/

cchaos

Thanks for looking into it @1Copenut.

I think we'll need to do more than just wrap with a relative div and shift some actual styles around. Specifically I'm looking at things that consumers may want to customize like sizing. Here's an example in Kibana that will break with this implementation:

https://github.com/elastic/kibana/blob/main/x-pack/plugins/cases/public/components/create/owner_selector.tsx#L46-L49

cchaos · 2022-01-11T19:08:18Z

src/components/key_pad_menu/key_pad_menu_item.tsx


    return (
      <EuiBetaBadge
+        aria-label={betaItemLabel}


This beta label can't be hard-coded because it doesn't always actually mean "Beta".

Suggested change

aria-label={betaItemLabel}

aria-label={`${label} ${betaBadgeLabel}`}

cchaos · 2022-01-11T19:15:02Z

src/components/key_pad_menu/key_pad_menu_item.tsx

+      >
+        {renderContent()}
+      </Element>
+      {betaBadgeLabel && !checkable && renderBetaBadge()}


The betaBadgeLabel check is already in renderBetaBadge() so I'd just move the !checkable check into there as well, so you can simplify this to just:

Suggested change

{betaBadgeLabel && !checkable && renderBetaBadge()}

{renderBetaBadge()}

cchaos · 2022-01-11T19:16:07Z

src/components/key_pad_menu/key_pad_menu_item.tsx

  const renderContent = () => (
    <span className="euiKeyPadMenuItem__inner">
-      {checkable ? renderCheckableElement() : renderBetaBadge()}
+      {checkable && renderCheckableElement()}


The checkable check is already in renderCheckableElement()

Suggested change

{checkable && renderCheckableElement()}

{renderCheckableElement()}

* Refactoring for variable container width. * Adding two spans to ensure positioning and variable width to container.

1Copenut · 2022-01-13T22:49:50Z

src/components/key_pad_menu/key_pad_menu_item.tsx

-    >
-      {renderContent()}
-    </Element>
+    <span className="euiKeyPadMenuItem__outer">


I thought a lot about @cchaos comments the past couple of days. After a number of false starts, I settled on this solution. The euiKeyPadMenuItem__outer class allows consumers to extend the component width easily. The euiKeyPadMenuItem__positioned class sets a relative position reference and a max-width so buttons, links, and labels maintain the 96px width. Beta badges will also maintain their position next to the button as separate focusable elements.

kibanamachine · 2022-01-13T23:21:51Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_5508/

cchaos · 2022-01-14T21:17:10Z

@1Copenut I've opened a new PR to showcase an idea I had for this. I couldn't do a PR against this branch because there were too many undos to get to the original and I couldn't make one against your fork because your main branch was too out of date. So here it is: #5541

1Copenut · 2022-01-19T14:32:01Z

Closing this PR for a more robust solution Caroline merged here #5541

Refactoring EuiKeypadMenuItem beta badge for improved accessibility

3258086

* Moved the EuiBetaBadge outside the parent button * Updated the absolute positioning of the beta badge to match current * Added role="button" to beta badge span for more consistent SR read out

1Copenut added accessibility accessibility - screen reader labels Dec 28, 2021

1Copenut requested review from breehall, cchaos, cee-chen and elizabetdev December 28, 2021 20:41

1Copenut self-assigned this Dec 28, 2021

Adding a changelog entry for issue 5290.

c107d41

1Copenut commented Dec 28, 2021

View reviewed changes

1Copenut marked this pull request as ready for review December 28, 2021 20:48

chandlerprall suggested changes Dec 29, 2021

View reviewed changes

Refactoring CSS and adding JS guard check

8723f28

1Copenut requested a review from chandlerprall December 29, 2021 20:05

1Copenut changed the title ~~Refactoring EuiKeypadMenuItem beta badge for improved accessibility~~ Refactoring EuiKeyPadMenuItem beta badge for improved accessibility Dec 29, 2021

chandlerprall suggested changes Dec 30, 2021

View reviewed changes

1Copenut added 2 commits January 4, 2022 10:03

Refactoring ARIA label for localization.

1f09ad2

Fixing a changelog conflict.

6f679f3

1Copenut requested a review from chandlerprall January 4, 2022 16:08

Refactoring to use a relatively positioned container.

3744698

1Copenut commented Jan 11, 2022

View reviewed changes

cchaos suggested changes Jan 11, 2022

View reviewed changes

Refactoring for variable width parent, and fixed element width

ba06212

* Refactoring for variable container width. * Adding two spans to ensure positioning and variable width to container.

1Copenut requested a review from cchaos January 13, 2022 22:44

Removing unused variable and comments

a7d44b5

1Copenut commented Jan 13, 2022

View reviewed changes

cchaos mentioned this pull request Jan 14, 2022

[EuiKeyPadMenuItem] Moves beta badge tooltip props to a wrapping EuiToolTip #5541

Merged

8 tasks

1Copenut closed this Jan 19, 2022

1Copenut deleted the feature/tpierce-eui-5290 branch January 19, 2022 14:32

	aria-label={betaItemLabel}
	aria-label={`${label} ${betaBadgeLabel}`}

	{betaBadgeLabel && !checkable && renderBetaBadge()}
	{renderBetaBadge()}

	{checkable && renderCheckableElement()}
	{renderCheckableElement()}

Conversation

1Copenut commented Dec 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

1Copenut left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Dec 28, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1Copenut Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kibanamachine commented Dec 29, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 4, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 11, 2022

Uh oh!

cchaos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cchaos Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kibanamachine commented Jan 13, 2022

Uh oh!

cchaos commented Jan 14, 2022

Uh oh!

1Copenut commented Jan 19, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

1Copenut commented Dec 28, 2021 •

edited

Loading

1Copenut Jan 6, 2022 •

edited

Loading

cchaos left a comment •

edited

Loading

cchaos Jan 11, 2022 •

edited

Loading